Development/websocket #1796

msieben · 2024-11-22T13:32:03Z

No description provided.

…m item - Rename 'WEBSERVER' to 'WEBSERVICE' as both client and server utilize state information - Unsollicited 'Pong' may act as a heart beat

…st_websocket'

…ructure are not yet contructed, and, improve SSL read and write use - SSL read and write methods should only be used if the handshake is successfull - Do not return error values for SSL read and write methods as number of read or written bytes

…en opened but context has not been contructed.

…arily) propagate to SSL structure after construction. - Context should exist - Options should be set

…hostname.

…n 'Handler::Initialize'

…ocket/WebSocketLink] : cherry pick from 'master'

Context settings do not (necessarily) propagate to the SSL structure

…SL read and write Only read or written bytes have values >= 0, otherwise it indicates an error and does not reflect the actual amount

…and sll structure in 'Update'

Whoops ....

Whoops

pwielders · 2024-11-25T17:41:02Z

Source/cryptalgo/SecureSocketPort.h

@@ -72,19 +72,26 @@ namespace Crypto {
            };

        public:
+            // Type of underlying socket


This enum, together with the enum class context_t split up into server and client do not add any functionality. They are currently used to "determine" the initial state of the _handshaking in the handler but the handler, in the initialize will override the set state, by definition, through the IsOpen() state, which indeed is the only valid way to determine if you are a server, so why all this additional code?
I suggest setting the initial state at ERROR and if during normal runtime you use this state and it is still ERROR, assert where you would not expect it..

Good observation. It was not the intended goal to have it overwritten.

pwielders · 2024-11-25T17:43:48Z

Source/cryptalgo/SecureSocketPort.h

-                , _handShaking(CONNECTING) {
-            }
+                , _handShaking(type == sockettype_t::CLIENT_SOCKET ? CONNECTING : ACCEPTING)
+                , _type{type}


As the sockettype_t type -> _type is not used anwhere, just drop it, it just confuses..

pwielders · 2024-11-25T17:46:31Z

Source/cryptalgo/SecureSocketPort.h

                : Core::SocketPort(args...)
                , _parent(parent)
                , _context(nullptr)
                , _ssl(nullptr)
                , _callback(nullptr)
-                , _handShaking(CONNECTING) {
-            }
+                , _handShaking(type == sockettype_t::CLIENT_SOCKET ? CONNECTING : ACCEPTING)


_handShaking is properly initialized during the Initialize() call t CONNECTING or ACCEPTING, based on the state of the socket if it is being called. Suggest to set it to ERROR here to assert in the INITIALIZE if, for example, it does not have this vale if we "Initialize()". Making sure the Initialize is called only once..

It was not the intended goal as mentioned here #1796 (comment)

Not the state of the socket as in open or closed. The intent is to differentiate between and identify the owner of the socket: a server using the accept method to construct it, or, a client that creates it to connect to a server, Handler::ACCEPTING, Handler::CONNECTING respectively.

pwielders · 2024-11-25T17:47:30Z

Source/cryptalgo/SecureSocketPort.h

+                   CLIENT_SOCKET
+                ,  SERVER_SOCKET
+            };
+
            Handler(Handler&&) = delete;
            Handler(const Handler&) = delete;
            Handler& operator=(const Handler&) = delete;


Please add the:
Handler& operator=(Handler&&) = delete;
for upgrading code we should add it now...

pwielders · 2024-11-25T17:49:44Z

Source/cryptalgo/SecureSocketPort.h

@@ -107,6 +114,7 @@ namespace Crypto {

            // Signal a state change, Opened, Closed or Accepted
            void StateChange() override {
+//                ASSERT(_context != nullptr && _ssl != nullptr);


Interesting that this ASSERT fires... Maybe during the Close Sequence? Please investigate and remove the assert or make sure that under normal conditions it does not fire (IsClosed() == false) || ((_context != nullptr) && (_ssl != nullptr)));

This is interesting. In a previous draft PR you did not wanted such solution but promoted the use of ASSERT. Update() already includes a check and ASSERT.

pwielders · 2024-11-25T17:50:02Z

Source/cryptalgo/SecureSocketPort.h

@@ -135,17 +143,31 @@ namespace Crypto {
            void* _ssl;
            IValidator* _callback;
            mutable state _handShaking;
+            const sockettype_t _type;


As described above, I think we can drop this..

Are you sure? _handShaking is initially used to identify the server or client side of the connection, but will loose that information once connected or in error.

rdkcmf-jenkins · 2025-01-07T11:02:35Z

Source/cryptalgo/SecureSocketPort.cpp


-    uint32_t result = SSL_write(_ssl, buffer, length);
+    ASSERT(    !IsNarrowing(std::numeric_limits<uint32_t>::max(), std::numeric_limits<time_t>::max())


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

rdkcmf-jenkins · 2025-01-07T11:02:36Z

Source/cryptalgo/SecureSocketPort.cpp

-    if (_handShaking != OPEN) {
-        Update();
-    }
+    ASSERT(    !IsNarrowing(std::numeric_limits<uint32_t>::max(), std::numeric_limits<suseconds_t>::max())


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

rdkcmf-jenkins · 2025-01-07T13:12:36Z

Source/cryptalgo/SecureSocketPort.cpp

+    using common_time_t VARIABLE_IS_NOT_USED = std::common_type<time_t, uint32_t>::type;
+    using common_seconds_t VARIABLE_IS_NOT_USED = std::common_type<suseconds_t, uint32_t>::type;
+
+    ASSERT(static_cast<common_time_t>(std::numeric_limits<time_t>::max()) >= static_cast<common_time_t>(waitTime));


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical operand of "!".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

rdkcmf-jenkins · 2025-01-07T13:12:36Z

Source/cryptalgo/SecureSocketPort.cpp

+    using common_seconds_t VARIABLE_IS_NOT_USED = std::common_type<suseconds_t, uint32_t>::type;
+
+    ASSERT(static_cast<common_time_t>(std::numeric_limits<time_t>::max()) >= static_cast<common_time_t>(waitTime));
+    ASSERT(static_cast<common_time_t>(std::numeric_limits<suseconds_t>::max()) >= static_cast<common_time_t>(waitTime));


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical operand of "!".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

rdkcmf-jenkins · 2025-01-07T13:55:28Z

Source/cryptalgo/SecureSocketPort.cpp

-    if (_handShaking != OPEN) {
-        Update();
-    }
+    ASSERT(    !IsNarrowing(std::numeric_limits<uint32_t>::max(), std::numeric_limits<suseconds_t>::max())


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

rdkcmf-jenkins · 2025-01-07T14:52:08Z

Source/cryptalgo/SecureSocketPort.cpp


-    uint32_t result = SSL_write(_ssl, buffer, length);
+    ASSERT(    !IsNarrowing(std::numeric_limits<uint32_t>::max(), std::numeric_limits<time_t>::max())


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

rdkcmf-jenkins · 2025-01-07T14:52:08Z

Source/cryptalgo/SecureSocketPort.cpp

-    if (_handShaking != OPEN) {
-        Update();
-    }
+    ASSERT(    !IsNarrowing(std::numeric_limits<uint32_t>::max(), std::numeric_limits<suseconds_t>::max())


Coverity Issue - Operands don't affect result

"9223372036854775807L /* static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(std::numeric_limits::max()) */ >= static_cast<Thunder::Crypto::SecureSocketPort::Handler::Open(unsigned int)::common_time_t>(waitTime)" is always true regardless of the values of its operands. This occurs as the logical second operand of "&&".

Medium Impact, CWE-569
CONSTANT_EXPRESSION_RESULT

* [Tests] : Reduce compiler warnings * [Tests/unit/core] : amend '0badc'

…te' to 'CertificateStore'

… (compile-time) between 'client' and 'server' type sockets Compile-time differentiation allows for use case available signatures

rdkcmf-jenkins · 2025-01-09T09:09:38Z

Source/cryptalgo/SecureSocketPort.cpp

+    return 0; // 0 - Failurre, 1 - OK
+}
+
+    template <typename TYPE = std::false_type>


Coverity Issue - Parse recovery warning

a default template argument cannot be specified on the definition of a member of a class template outside the template

Low Impact, CWE-none
RW.DEFAULT_ARG_ON_MEMBER_DECL

…fety

msieben and others added 22 commits November 6, 2024 15:23

[WebSocket/WebSocketLink] : Enable unsollicited 'Pong' and rename enu…

6f4705c

…m item - Rename 'WEBSERVER' to 'WEBSERVICE' as both client and server utilize state information - Unsollicited 'Pong' may act as a heart beat

[WebSocket/WebSocketLink] : METROL-1087

aa8c25d

[WebSocket/WebSocketLink] : rename forgotten 'WEBSERVER' to 'WEBSERVICE'

e923fcf

Merge branch 'master' into development/websocket

43c3174

[Tests/unit/core] : add 'test_websocket'

4769766

[Tests/unit/core] : Add missing VERBOSE preprocessor directive to 'te…

45bfec5

…st_websocket'

[Tests/unit/core] : Add 'SecureSocketPort' tests to 'test_websocket'

a206e19

[Source/cryptalgo/SecureSocketPort] : (Re)initialize if socket has be…

d9d1dd4

…en opened but context has not been contructed.

[Source/cryptalgo/SecureSocketPort] : Context settings do not (necess…

41fde68

…arily) propagate to SSL structure after construction. - Context should exist - Options should be set

[Source/cryptalgo/SecureSocketPort] : check return value for setting …

efe82e0

…hostname.

[Source/cryptalgo/SecureSocketPort] : improve return value checking i…

4a8b4b1

…n 'Handler::Initialize'

[Source/cryptalgo/SecureSocketPort] : correct check for invalid context

bdaff29

[Source/core/SocketPort Source/cryptalgo/SecureSocketPort Source/webs…

25634c6

…ocket/WebSocketLink] : cherry pick from 'master'

[Source/cryptalgo/SecureSocketPort] : use the generic method

f6e297e

[Source/cryptalgo/SecureSocketPort] : Re-order creation of structures

980e387

Context settings do not (necessarily) propagate to the SSL structure

[Source/cryptalgo/SecureSocketPort] : do not return error value for S…

25a61a3

…SL read and write Only read or written bytes have values >= 0, otherwise it indicates an error and does not reflect the actual amount

[Source/cryptalgo/SecureSocketPort] : add assert for invalid context …

d351981

…and sll structure in 'Update'

Merge branch 'master' into development/websocket

080e84a

Delete Source/cryptalgo/SecureSocketPort.cpp.modified

03a69dc

Whoops ....

Delete Source/cryptalgo/SecureSocketPort.h.modified

a596018

Whoops

Delete Source/cryptalgo/SecureSocketPort.modifications

f7dbcce

Whoops

msieben requested review from pwielders and MFransen69 November 22, 2024 13:55

pwielders reviewed Nov 25, 2024

View reviewed changes

msieben requested a review from rdkcmf-jenkins January 7, 2025 10:47

rdkcmf-jenkins reviewed Jan 7, 2025

View reviewed changes

msieben force-pushed the development/websocket branch from ad058f5 to 1e7c32b Compare January 7, 2025 13:02

rdkcmf-jenkins reviewed Jan 7, 2025

View reviewed changes

msieben force-pushed the development/websocket branch 2 times, most recently from 150d47d to 8d44d48 Compare January 7, 2025 13:42

rdkcmf-jenkins reviewed Jan 7, 2025

View reviewed changes

msieben force-pushed the development/websocket branch 2 times, most recently from 20cea57 to 52f07c9 Compare January 7, 2025 14:36

msieben requested a review from rdkcmf-jenkins January 7, 2025 14:50

rdkcmf-jenkins reviewed Jan 7, 2025

View reviewed changes

msieben added 4 commits January 7, 2025 15:15

Development/buildwarnings (#1821)

cf8383a

* [Tests] : Reduce compiler warnings * [Tests/unit/core] : amend '0badc'

[Source/core/SecureSocketPort] : CWE-569

25d16c7

[Tests/unit/core] : Uncomment 'VERBOSE' flags

4829f72

[Source/cryptalgo/SecureSocketPort] : CWE-569

2f2be2d

msieben force-pushed the development/websocket branch from 52f07c9 to 2f2be2d Compare January 7, 2025 15:20

msieben added 3 commits January 7, 2025 15:28

Merge branch 'master' into development/websocket

cd54ba8

[Source/cryptalgo/SecureSocketPort / Tests/unit/core] : Move 'IValida…

da99b1e

…te' to 'CertificateStore'

[Source/cryptalgo/SecureSocketPort / tests/unit/core] : Differentiate…

1572286

… (compile-time) between 'client' and 'server' type sockets Compile-time differentiation allows for use case available signatures

rdkcmf-jenkins reviewed Jan 9, 2025

View reviewed changes

msieben added 10 commits January 9, 2025 09:16

--amend

06a2e31

[Source/cryptalgo/SecureSocketPort] : disable default constructor

48b5599

[Source/cryptalgo/SecureSocket / Tests/unit/core] : Improve thread sa…

2299da6

…fety

[Tests/unit/core] : Re-enable disabled tests in 'test_websocket'

23f9df1

[Tests/unit/core] : Re-enable 'all' tests

a29a0b5

[Tests/unit/core] : add payload tests to 'test_websocket'

c96074a

Merge branch 'master' into development/websocket

d9804c5

[cryptalgo/SecureSocketPort] : indicate object can be move from

4008468

[cryptalgo/SecureSocketPort] : improve use of list

928bccd

[cryptalgo/SecureSocketPort] : remove comment

82bdb79

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Development/websocket #1796

Development/websocket #1796

msieben commented Nov 22, 2024

pwielders Nov 25, 2024 •

edited

Loading

msieben Nov 26, 2024

pwielders Nov 25, 2024

pwielders Nov 25, 2024

msieben Nov 26, 2024

msieben Nov 26, 2024

pwielders Nov 25, 2024

pwielders Nov 25, 2024

msieben Nov 26, 2024

pwielders Nov 25, 2024

msieben Nov 26, 2024

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 7, 2025

rdkcmf-jenkins Jan 9, 2025


		uint32_t result = SSL_write(_ssl, buffer, length);
		ASSERT( !IsNarrowing(std::numeric_limits<uint32_t>::max(), std::numeric_limits<time_t>::max())

Development/websocket #1796

Are you sure you want to change the base?

Development/websocket #1796

Conversation

msieben commented Nov 22, 2024

pwielders Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Operands don't affect result

Choose a reason for hiding this comment

Coverity Issue - Parse recovery warning

pwielders Nov 25, 2024 •

edited

Loading